feat: use full chart card for KPIs on member page #368
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request ReviewSummaryThis PR successfully replaces the compact ✅ Positive Aspects
🔍 Areas for Consideration1. Height Consistency Issue (Medium Priority)The placeholder "KPI data loading..." state now has a hardcoded height ( // Current placeholder
<div className="... flex h-[420px] items-center justify-center ...">
// But ReadOnlyMetricCard has no height constraint
<div className="relative">
<DashboardMetricChart ... />Suggestion: Consider either:
2. Missing Error Handling Context (Low Priority)The // In ReadOnlyMetricCard
{metric.lastError && !metric.refreshStatus && (
<Badge variant="destructive" ...>Error</Badge>
)}Question: Does the 3. Test Coverage (Low Priority)No tests exist for this component. While not blocking, consider adding basic tests for:
🎯 Code Quality Assessment
📋 Minor Suggestions
✨ Overall AssessmentRecommendation: Approve with minor suggestions ✅ This is a solid, focused PR that improves the user experience on the member page. The changes are clean, follow existing patterns, and don't introduce any significant issues. The height consistency concern is worth double-checking, but it's not a blocker. The removal of the unused 🚀 Next Steps (Optional)After merge, consider:
LGTM 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/app/member/[id]/_components/role-kpi-pair.tsx:
- Line 50: The placeholder div uses a hardcoded Tailwind height class h-[420px];
replace this with a CSS variable so the metric card height is shared with
ReadOnlyMetricCard/DashboardMetricChart: define --metric-card-height: 420px in
your global CSS or theme config (e.g., :root or globals.css) and change the JSX
className to use h-[var(--metric-card-height)] (and update the same replacement
inside ReadOnlyMetricCard / DashboardMetricChart) so both components read the
single source of truth for height.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/app/member/[id]/_components/team-section.tsxsrc/components/role/role-card.tsx
💤 Files with no reviewable changes (1)
- src/app/member/[id]/_components/team-section.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
Use TypeScript 5.9 with strict type checking for all frontend and backend code
Files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
src/**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Prefer Server Components for initial data fetching; use Client Components ('use client') only for interactivity
Files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
src/**/*/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Client Components must use
import { api } from '@/trpc/react'for standard HTTP/Hooks wrapper
Files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Use Tailwind CSS 4 for styling with shadcn/ui and Radix UI primitive components
Files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
src/app/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
src/app/**/*.tsx: Use the dual tRPC API pattern: direct calls in Server Components (api.team.getById) for 10x faster performance, and React hooks in Client Components (api.team.getById.useQuery)
Use getUserDisplayName(userId, members) utility (client-side sync) from @/lib/helpers/get-user-name for displaying user names in components
Files:
src/app/member/[id]/_components/role-kpi-pair.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use @trivago/prettier-plugin-sort-imports with inline type imports for import organization
Files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
src/components/**/*.tsx
📄 CodeRabbit inference engine (GEMINI.md)
Place colocated components in
_components/folders next to their parent componentUse shadcn/ui components from src/components/ui/; add new components via CLI: npx shadcn@latest add [component-name]
Files:
src/components/role/role-card.tsx
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/components/dashboard-metric-card.tsx,src/app/dashboard/[teamId]/_components/public-dashboard-metric-card.tsx : Dashboard metric cards are duplicated with public variant. Consolidate into single component with `readOnly` mode prop instead of maintaining separate components.
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/_components/role-node.tsx,src/app/teams/[teamId]/_components/public-role-node.tsx : These role node components are 75% identical and should be consolidated. Extract shared `RoleNodeTemplate` component with `isEditable` prop to DRY up the code.
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/teams/[teamId]/_components/**/*.tsx : Implement cache-first node pattern for role nodes: store only roleId in node data, fetch display data from TanStack Query cache using useRoleData hook
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-12-20T22:12:00.576Z
Learning: Applies to src/app/teams/[teamId]/**/*.tsx : React Flow nodes must store minimal data (e.g., just `roleId`); fetch full Role data from TanStack Query cache in the Node component to keep canvas and sidebars in sync
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/hooks/**/*.ts : For role-metric cache updates, optimistically update both role cache (role.getByTeamId) and dashboard cache (dashboard.getDashboardCharts) to maintain UI consistency
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/dashboard/[teamId]/**/*.{ts,tsx} : Dashboard cache updates for role-metric assignments must update both `role.getByTeamId` and `dashboard.getDashboardCharts` caches during mutations. Use onMutate for optimistic updates on both caches, then invalidate both on success.
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/**/*.{ts,tsx} : Cache-first node system: Role nodes store only `roleId` in node.data. Display data must be fetched from TanStack Query cache using the `useRoleData()` hook which queries the `role.getByTeamId` procedure.
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/components/dashboard-metric-card.tsx,src/app/dashboard/[teamId]/_components/public-dashboard-metric-card.tsx : Dashboard metric cards are duplicated with public variant. Consolidate into single component with `readOnly` mode prop instead of maintaining separate components.
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/_components/role-node.tsx,src/app/teams/[teamId]/_components/public-role-node.tsx : These role node components are 75% identical and should be consolidated. Extract shared `RoleNodeTemplate` component with `isEditable` prop to DRY up the code.
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-20T22:12:00.576Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-12-20T22:12:00.576Z
Learning: Applies to src/app/teams/[teamId]/**/*.tsx : React Flow nodes must store minimal data (e.g., just `roleId`); fetch full Role data from TanStack Query cache in the Node component to keep canvas and sidebars in sync
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/hooks/**/*.ts : For role-metric cache updates, optimistically update both role cache (role.getByTeamId) and dashboard cache (dashboard.getDashboardCharts) to maintain UI consistency
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/dashboard/[teamId]/**/*.{ts,tsx} : Dashboard cache updates for role-metric assignments must update both `role.getByTeamId` and `dashboard.getDashboardCharts` caches during mutations. Use onMutate for optimistic updates on both caches, then invalidate both on success.
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/teams/[teamId]/_components/**/*.tsx : Implement cache-first node pattern for role nodes: store only roleId in node data, fetch display data from TanStack Query cache using useRoleData hook
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/teams/[teamId]/**/*.{ts,tsx} : Cache-first node system: Role nodes store only `roleId` in node.data. Display data must be fetched from TanStack Query cache using the `useRoleData()` hook which queries the `role.getByTeamId` procedure.
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-20T22:12:00.576Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-12-20T22:12:00.576Z
Learning: Applies to src/app/teams/[teamId]/**/*.tsx : Avoid modifying `enrichNodesWithRoleData` flow without understanding the complete canvas serialization logic for saving/loading React Flow nodes to the database
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsxsrc/components/role/role-card.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/(metric|dashboard)/**/*.tsx : Use three-stage metrics transformation: API → DataPoints (DataIngestionTransformer), DataPoints → ChartConfig (ChartTransformer), ChartConfig → UI (DashboardMetricChart)
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/(teams|org)/**/*.tsx : Use shared MembersList component at src/components/member/member-list.tsx for displaying members in canvas sidebar and org page; it includes getMemberDisplayInfo() utility for initials/name logic
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Use shared MetricDialogBase component from base/ for all metric dialog implementations
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T21:27:04.176Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T21:27:04.176Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Create metric dialogs with pattern: [Provider]MetricDialog.tsx (dialog wrapper) + [Provider]MetricContent.tsx (form content) in src/app/metric/_components/[provider]/, then register in index.ts
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/metric/_components/**/*.tsx : Metric dialog components should follow the pattern: [Provider]MetricDialog.tsx (dialog wrapper) and [Provider]MetricContent.tsx (form content). Register in src/app/metric/_components/index.ts and inherit from shared MetricDialogBase.
Applied to files:
src/app/member/[id]/_components/role-kpi-pair.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/hooks/use-optimistic-role-update.ts : The `use-optimistic-role-update` hook provides shared logic for all role mutations with proper cache layer handling for TanStack Query and Prisma Accelerate. Use this hook in canvas-specific wrappers to add markDirty side effects.
Applied to files:
src/components/role/role-card.tsx
📚 Learning: 2025-12-29T12:52:42.935Z
Learnt from: CR
Repo: drifter089/orgOS PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T12:52:42.935Z
Learning: Applies to src/app/**/*role*mutation*.{ts,tsx} : For role mutations, use cache pipeline with two operations: (1) onMutate for optimistic update, (2) onSuccess with both setData(updatedRole) using server response AND invalidate() for background refresh. Always use server response data in setData before invalidate to ensure Prisma Accelerate cache consistency.
Applied to files:
src/components/role/role-card.tsx
🧬 Code graph analysis (1)
src/app/member/[id]/_components/role-kpi-pair.tsx (2)
src/components/role/role-card.tsx (1)
RoleCard(330-330)src/app/dashboard/[teamId]/_components/dashboard-metric-card.tsx (1)
ReadOnlyMetricCard(125-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (4)
src/app/member/[id]/_components/role-kpi-pair.tsx (3)
11-14: Clean prop interface simplification.Removing the unused
teamIdprop fromRoleKpiPairsimplifies the component API and correctly reflects thatReadOnlyMetricCarddoesn't require team context for rendering.Also applies to: 16-16
39-39: LGTM! Minor spacing adjustment.The grid gap change from
gap-3togap-4provides slightly more breathing room between the role card and KPI card.
3-3: Type compatibility verified — consolidation pattern is correct.The replacement of
KpiCardwithReadOnlyMetricCardcorrectly implements the dashboard metric card consolidation. BothDashboardChart(defined in role-kpi-pair.tsx) andDashboardChartWithRelations(expected by ReadOnlyMetricCard) are the same type:RouterOutputs["dashboard"]["getDashboardCharts"][number]. The component signature matches perfectly—ReadOnlyMetricCard requires onlydashboardChart, eliminating the need for theteamIdprop. This aligns with the consolidation pattern: a single component with read-only rendering instead of maintaining separate variants.src/components/role/role-card.tsx (1)
184-185: LGTM! Appropriate conditional hover styling.The conditional
hover:shadow-lgcorrectly removes hover animations when cards are in display-only (readOnly) mode, improving the UX by signaling that these cards are not interactive. Theselectedstate and other styling remain independent and functional.
|
|
||
| {hasMetricButNoChart && ( | ||
| <div className="border-border/60 text-muted-foreground flex items-center justify-center rounded-lg border border-dashed p-6"> | ||
| <div className="border-border/60 text-muted-foreground flex h-[420px] items-center justify-center rounded-xl border border-dashed p-6"> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the location of ReadOnlyMetricCard and related components
fd -t f 'dashboard-metric' src/ | head -20
fd -t f 'role-kpi-pair' src/Repository: drifter089/orgOS
Length of output: 374
🏁 Script executed:
# Search for ReadOnlyMetricCard definition
rg -l 'ReadOnlyMetricCard' src/Repository: drifter089/orgOS
Length of output: 366
🏁 Script executed:
# Get more context about the file under review
cat -n src/app/member/\[id\]/_components/role-kpi-pair.tsx | head -60Repository: drifter089/orgOS
Length of output: 2158
🏁 Script executed:
# Read the dashboard-metric-card.tsx file
cat -n src/app/dashboard/\[teamId\]/_components/dashboard-metric-card.tsxRepository: drifter089/orgOS
Length of output: 6447
🏁 Script executed:
# Also check the DashboardMetricChart component since it's likely used by ReadOnlyMetricCard
cat -n src/app/dashboard/\[teamId\]/_components/dashboard-metric-chart.tsxRepository: drifter089/orgOS
Length of output: 18496
Use a shared CSS variable for the metric card height instead of hardcoding h-[420px] in two places.
Both the placeholder in this file and ReadOnlyMetricCard (via DashboardMetricChart) currently use h-[420px], so heights match. However, this magic number is duplicated across the codebase. Extracting it to a CSS variable in your globals or theme config ensures future changes to the metric card height automatically propagate everywhere, preventing layout shifts during maintenance.
🤖 Prompt for AI Agents
In @src/app/member/[id]/_components/role-kpi-pair.tsx at line 50, The
placeholder div uses a hardcoded Tailwind height class h-[420px]; replace this
with a CSS variable so the metric card height is shared with
ReadOnlyMetricCard/DashboardMetricChart: define --metric-card-height: 420px in
your global CSS or theme config (e.g., :root or globals.css) and change the JSX
className to use h-[var(--metric-card-height)] (and update the same replacement
inside ReadOnlyMetricCard / DashboardMetricChart) so both components read the
single source of truth for height.
Summary
Replace the compact KPI card on the member page with the full dashboard chart card that includes actual chart visualizations. Also removes hover animations from cards on the member page since they are display-only.
Changes
KpiCardwithReadOnlyMetricCardon member page for full chart displayRoleCardwhen in read-only modeteamIdprop fromRoleKpiPaircomponentSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.